Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integers #877

Merged
merged 11 commits into from Oct 30, 2017
Merged

Integers #877

merged 11 commits into from Oct 30, 2017

Conversation

martinthomson
Copy link
Member

@martinthomson martinthomson commented Oct 16, 2017

This isn't quite ready - I have to make some changes in the HTTP doc, but I thought that I'd share it now so that we have a better location for discussion.

This proposal replaces all integer encodings in frames with a unified variable-length encoding format. It simplifies the encoding considerably, most noticeably in the ACK frame, which I have largely rewritten.

Closes #595, #109.

This proposal replaces all integer encodings in frames with a unified
variable-length encoding format.  It simplifies the encoding considerably, most
noticeably in the ACK frame, which I have largely rewritten.

This change increases the number of streams to 2^62, but reduces the number of
packets and octets per stream to that same number.  Both are very large values
anyway.  I kept the 2^10 scaling on the connection-level flow control window,
so that goes to 2^72 now.

The cost for ACK is a negligible decrease in efficiency for larger gaps or
ACK blocks (mainly between 64 and 255 in length).  This is counteracted by an
increase in efficiency if there are occasional ranges or gaps larger than 255,
which were previously very difficult to encode correctly.

As previously discussed, the timestamp format is removed.  In its place, we use
the same integer encoding, but with a scaling value, or exponent, that is
signaled in transport parameters, that should allow common ACK delays to be
expressed in fewer octets (an exponent of 0 only allows for 16ms to be encoded
in two octets, but the default exponent is 8, allowing up to 128ms of ACK delay
to be encoded on two octets with an 8us resolution).

I haven't made any changes for HTTP yet, I plan to do that in a separate PR.
martinthomson added a commit that referenced this pull request Oct 18, 2017
martinthomson added a commit that referenced this pull request Oct 18, 2017
This didn't turn out to be that disruptive.  I'm somewhat worried that I missed
something actually.  Given that the changes are small, I think that I would
prefer this over #887 for #877.

The most interesting change here is that extension frame types that mention
Stream IDs will have to change.  I am not aware of any proposed thus far, so
maybe it's not too much of a cost to bear.  It further confirms my view that we
shouldn't be using Stream IDs at this layer anyway.

Based on this, I don't see any need for a middle-ground change.  The biggest
cost here is in taking the larger Stream ID space, not in taking the integer
encoding.
Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the ack section, some editorial suggestions.

field.
The ACK Block Section consists of alternating ACK Blocks and Gaps. A First Ack
Block is followed by a variable number of alternating Gap and Additional ACK
Blocks. The number of Additional ACK Blocks and Gaps is determined by the ACK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd always say "Gaps and Blocks" instead of "Blocks and Gaps" since that's the order in the previous sentence.

the First ACK Block length is present in this section. Otherwise, the Num Blocks
field indicates how many additional blocks follow the First ACK Block Length
field.
The ACK Block Section consists of alternating ACK Blocks and Gaps. A First Ack
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd note that they're in descending order immediately, since it's been a source of confusion. ie: "The ACK Block Section consists of alternating ACK Blocks and Gaps in descending packet number order."

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
~~~
{: #ack-block-format title="ACK Block Section"}

The fields in the ACK Block Section are:
Each ACK Block acknowledges a contiguous range of packets by indicating the
number of packets that acknowledged preceding the largest packet number in that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "that" before acknowledged.

: An unsigned packet number delta that indicates the number of contiguous
packets being acknowledged starting after the end of the previous gap.
Repeated "Num Blocks" times.
: A variable-length indicating the number of contiguous packets preceding the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

variable-length integer?


#### Time Format
: A non-zero, variable-length integer indicating specifying the number of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zero is ok now, correct? And I think the subsequent text needs to be updated to match the above sentence: "The number of packets in the gap is one higher than the encoded value of the Gap Field."

packets preceding the largest packet number, as determined by the
preceding Gap.

If the repeated Gap and ACK Block fields do not end precisely at the end of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this paragraph is true, so should be removed?

The first byte in the stream has an offset of 0. The largest offset delivered
on a stream - the sum of the re-constructed offset and data length - MUST be
less than 2^64.
: A variable-sized integer specifying the byte offset in the stream for the data
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You use variable-length elsewhere


#### Time Format
: A variable-length integer indicating specifying the number of contiguous
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indicating specifying is redundant. Also, you alternate between using indicating and specifying in these sections. Is it worth consistently using one or the other?

Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with merging this, minus the one "indicating specifying" editorial nit I commented on.

martinthomson and others added 5 commits October 30, 2017 16:21
This didn't turn out to be that disruptive.  I'm somewhat worried that I missed
something actually.  Given that the changes are small, I think that I would
prefer this over #887 for #877.

The most interesting change here is that extension frame types that mention
Stream IDs will have to change.  I am not aware of any proposed thus far, so
maybe it's not too much of a cost to bear.  It further confirms my view that we
shouldn't be using Stream IDs at this layer anyway.

Based on this, I don't see any need for a middle-ground change.  The biggest
cost here is in taking the larger Stream ID space, not in taking the integer
encoding.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants